Skip to content

fix(firestore): avoid reusing busy clients for streams#7950

Open
akshatbaranwal wants to merge 2 commits intogoogleapis:mainfrom
akshatbaranwal:fix/request-stream-client-isolation-upstream
Open

fix(firestore): avoid reusing busy clients for streams#7950
akshatbaranwal wants to merge 2 commits intogoogleapis:mainfrom
akshatbaranwal:fix/request-stream-client-isolation-upstream

Conversation

@akshatbaranwal
Copy link
Copy Markdown

Summary

  • add an opt-in preferIdleClients path in ClientPool
  • use that mode for requestStream() so new streams reuse idle clients but do not pile onto already-busy ones
  • add pool regression tests for the new reuse behavior

Why

DocumentReference.get() and getAll() use the batchGetDocuments server-streaming path. If one stream stalls for a long time, the existing ClientPool strategy prefers the most-full client and can keep routing later low-concurrency reads back onto the same busy client.

This change is a defensive mitigation: request streams will reuse idle clients, but if all existing clients are already busy, they open a fresh client instead of piling onto a busy one.

Testing

  • cd handwritten/firestore && npx mocha -r ts-node/register/transpile-only dev/test/pool.ts --grep "preferIdleClients|re-uses idle instances when preferIdleClients|creates a new client when preferIdleClients|creates new instances as needed|re-uses idle instances"
  • cd handwritten/firestore && npx tsc -p . --noEmit --skipLibCheck

@akshatbaranwal akshatbaranwal requested a review from a team as a code owner April 2, 2026 20:33
@product-auto-label product-auto-label bot added the api: firestore Issues related to the Firestore API. label Apr 2, 2026
@dlarocque
Copy link
Copy Markdown
Contributor

@akshatbaranwal Does this change solve any open issues with this SDK?

@akshatbaranwal
Copy link
Copy Markdown
Author

Yes — this is the Firestore-side mitigation for #7960.

That issue describes how a single stalled REST-fallback server-streaming read (batchGetDocuments) can leave a ClientPool client checked out indefinitely, and because the pool prefers the most-full existing client, later low-concurrency reads keep reacquiring the same poisoned client until the process is restarted.

This PR doesn't try to fix the underlying transport stall (that's tracked separately in #7959). It's a defensive change in ClientPool: under the new preferIdleClients mode — which requestStream() opts into — a new stream reuses an idle client if one exists, but if every existing client is already busy it opens a fresh client rather than piling onto a busy one. That way one stuck stream can no longer attract every subsequent read.

Happy to scope it differently (e.g. stale-stream eviction instead of an opt-in reuse policy) if that's closer to what you'd prefer — question #1 in #7960 was explicitly asking for maintainer guidance on that.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

api: firestore Issues related to the Firestore API.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants